Serve non-html at documentation domain though El Proxito #6419

humitos · 2019-11-28T18:04:39Z

Download URLs from the footer call will be like,

Once the user hit these URLs the PDF/ePub/HTML will be serve directly at that URL by El Proxito --no redirect is performed.

This PR also includes a change in the El Proxito logic to serve any file as well. Instead of generating the path to the filename manually, the django-storage backend is used. This allow us to generate the same URL for non-html files from the FooterHTML view than from ServeDocsBase.

~~Also, the "Downloads" link that point to the Dashboard in the footer is removed.~~

Closes #5855
Closes #6100

Download URLs from the footer call will be like, - project-slug.readthedocs.io/_/downloads/pdf/project-slug/latest/ - docs.example.org/_/downloads/pdf/subproject-slug/latest/ Once the user hit these URLs the PDF/ePub/HTML will be serve directly at that URL by El Proxito --no redirect is performed. This commit also includes a change in the El Proxito logic to serve any file as well. Instead of generating the `path` to the filename manually, the django-storage backend is used. This allow us to generate the same URL for non-html files from the `FooterHTML` view than from `ServeDocsBase`.

docker/nginx/proxito.conf

docker/nginx/web.conf

readthedocs/proxito/views/serve.py

readthedocs/urls.py

This allows us to have more control on the storage class (return always True on `.exists()`) and only mock the class when we need a different behavior.

ericholscher

This is a good start, but again we need to not break backwards compat with these changes.

readthedocs/api/v3/serializers.py

readthedocs/projects/urls/public.py

readthedocs/proxito/views/serve.py

readthedocs/settings/test.py

readthedocs/urls.py

We need to keep this URL to support old links.

Also, re add the new download URL to the main site since it's required to generate the URL for the APIv3

…tml-same-domain

ericholscher · 2019-12-02T14:18:21Z

I'm wondering if it would be easier to just support both the dashboard & in-doc download URL's for a time. It seems like we're adding a lot of complexity here, when we really just want to be able to support both for a while. Once we have both working, we can figure out how to migrate the API and other paths we link to.

davidfischer

I'm wondering if it would be easier to just support both the dashboard & in-doc download URL's for a time.

I'm in favor of this because I suspect some people are linking to the old URL.

readthedocs/settings/base.py

humitos · 2019-12-03T12:22:56Z

@davidfischer @ericholscher I'm not sure to follow you. What are the changes I need to do in this PR to be accepted?

You both mentioned supporting both, old and new links. This PR won't change that after reverting the commit that removed the URL defined. On the other hand, if you are talking about the NGINX config (#6419 (comment)), I'm not sure it worth to have this in development since the only way to get there is manually writing the URL. We won't change this in the -ops repo.

davidfischer · 2019-12-03T23:14:37Z

What are the changes I need to do in this PR to be accepted?

I think we were both saying that the old URL (eg. https://readthedocs.org/projects/project-slug/downloads/pdf/latest/) should still work. Since we put that back and just removed the name= parameter, I think we should be ok.

ericholscher · 2019-12-04T11:55:18Z

Yea, as long as we don't break the old URL's I'm happy.

ericholscher

This is going in a good direction, but I still think the URLConf hack is the wrong approach. Let's either keep serving from the dashboard URL's for now, or just explicitly build the URL, instead of the current hack of adding a URLConf to the main site that we never want to serve.

docker/nginx/proxito.conf

docker/nginx/web.conf

readthedocs/api/v2/templates/restapi/footer.html

readthedocs/projects/models.py

readthedocs/projects/urls/public.py

…tml-same-domain

No need to strip and re-add a portion of the URL when proxying.

This reverts commit 7bc611d.

This reverts commit d4b1275.

There is no need for `full_path` attribute. It was not used anywhere and the path without the domain does not make sense and will be broken.

…tml-same-domain

ericholscher · 2019-12-16T11:38:40Z

This feels close, but I still don't like that we're serving the URL's on all proxito domains. Locally this works:

http://django-test-utils.community.dev.readthedocs.io/_/downloads/pdf/test-builds/latest/

Downloads the test-builds PDF on the test-utils domain :/

This all leads back to the weirdness around URL's and domains, but this is definitely not ready to ship as it is currently implemented. We need to have a better way of download this project's PDF, not just ship our old general "download any PDF endpoint" onto the doc domains.

readthedocs/projects/models.py

humitos · 2019-12-17T11:20:20Z

The code looks good and I think we agree we want this. Although, I'm blocking it because I suppose we want to have a discussion/talk about how the URLs will look like. We've talked already about this but we didn't make a decision yet.

…humitos/serve-non-html-same-domain

humitos · 2020-01-13T21:06:51Z

This PR is ready for re-review.

We decided to use these URLs to serve files for download:

docs.example.com/_/downloads/<lang>/<ver>/pdf/
docs.example.com/_/downloads/<alias>/<lang>/<ver>/pdf/

The latest commits implement this.

ericholscher

Looks like a good change, I think we're pretty much ready once the tests pass and we add a couple docstrings. 👍

readthedocs/projects/models.py

readthedocs/projects/views/public.py

readthedocs/proxito/urls.py

humitos · 2020-01-16T20:13:29Z

I'm merging this PR now. Downloads are ready to start being served by El Proxito. I've already opened the -ops PRs needed together with this change for the deploy.

humitos added 2 commits November 28, 2019 18:55

Remove "Downloads" link to dashboard from footer response

d4b1275

humitos requested review from ericholscher and davidfischer and removed request for ericholscher November 28, 2019 18:05

humitos commented Nov 28, 2019

View reviewed changes

humitos added 5 commits November 28, 2019 19:43

Add /media/ (storage bucket) to proxito test URLs (X-Accel-Redirect)

33735f4

Serve robots.txt using the storage backend

97b5667

Use a custom storage for tests

1a82333

This allows us to have more control on the storage class (return always True on `.exists()`) and only mock the class when we need a different behavior.

Adapt all APIv3 tests for the new URLs

65567e9

Update documentation with the new URLs

d622579

ericholscher reviewed Dec 2, 2019

View reviewed changes

humitos added 8 commits December 2, 2019 12:58

Use BuildMediaFileSystemStorageTest on required tests only

3f669bf

Re-add URL to serve non-html files from dashboard

b6fd7fd

We need to keep this URL to support old links.

Do not define /_/downloads/... URL in main site

6b97b56

Remove name= attribute from "project_download_media" URL

284286b

Also, re add the new download URL to the main site since it's required to generate the URL for the APIv3

Move test for download pdf file to el proxito

44acf5b

Skip test for URL without name= attribute defined

7bc611d

Merge remote-tracking branch 'origin/master' into humitos/serve-non-h…

bd5829d

…tml-same-domain

Style

809e4df

davidfischer reviewed Dec 3, 2019

View reviewed changes

readthedocs/settings/base.py Show resolved Hide resolved

ericholscher reviewed Dec 4, 2019

View reviewed changes

humitos added 3 commits December 4, 2019 19:43

Merge remote-tracking branch 'origin/master' into humitos/serve-non-h…

7d09d9c

…tml-same-domain

Proxy to the storage directly

f4d1842

No need to strip and re-add a portion of the URL when proxying.

Revert "Skip test for URL without name= attribute defined"

7926e57

This reverts commit 7bc611d.

ericholscher changed the title ~~Serve non-html at documentation domain~~ Serve non-html at documentation domain though El Proxito Dec 5, 2019

humitos added 3 commits December 5, 2019 16:11

Revert "Remove "Downloads" link to dashboard from footer response"

6c9a5b6

This reverts commit d4b1275.

Refactor get_production_media_url

f0ca138

There is no need for `full_path` attribute. It was not used anywhere and the path without the domain does not make sense and will be broken.

Fix tests

081cbe1

ericholscher removed the Status: blocked Issue is blocked on another issue label Dec 16, 2019

Merge remote-tracking branch 'origin/master' into humitos/serve-non-h…

0ed5185

…tml-same-domain

humitos commented Dec 16, 2019

View reviewed changes

readthedocs/projects/models.py Outdated Show resolved Hide resolved

humitos added the Status: blocked Issue is blocked on another issue label Dec 17, 2019

humitos added Accepted Accepted issue on our roadmap PR: work in progress Pull request is not ready for full review and removed Status: blocked Issue is blocked on another issue labels Jan 13, 2020

humitos added 5 commits January 13, 2020 20:11

Merge branch 'master' of github.com:readthedocs/readthedocs.org into …

3d73a36

…humitos/serve-non-html-same-domain

Use final discussed URLs for download files

bc2e06c

Add missing self in the method definition

5653a23

Remove old test

3502bad

Remove old comment

014643a

humitos removed the PR: work in progress Pull request is not ready for full review label Jan 13, 2020

ericholscher approved these changes Jan 14, 2020

View reviewed changes

readthedocs/projects/models.py Show resolved Hide resolved

readthedocs/projects/models.py Show resolved Hide resolved

readthedocs/projects/views/public.py Show resolved Hide resolved

readthedocs/proxito/urls.py Outdated Show resolved Hide resolved

humitos added 6 commits January 14, 2020 12:52

Add some docstrings

5e9b6c4

Split URL with optional <alias> in two

be6cf14

Adapt tests to match the new download URLs

c4da1b1

Use same permission logic in both URLs

e657ba4

Lint

6414fbd

More lint

36ec761

humitos merged commit d4e3538 into master Jan 16, 2020

humitos deleted the humitos/serve-non-html-same-domain branch January 16, 2020 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serve non-html at documentation domain though El Proxito #6419

Serve non-html at documentation domain though El Proxito #6419

humitos commented Nov 28, 2019 •

edited

Loading

ericholscher left a comment

ericholscher commented Dec 2, 2019

davidfischer left a comment

humitos commented Dec 3, 2019

davidfischer commented Dec 3, 2019

ericholscher commented Dec 4, 2019

ericholscher left a comment

ericholscher commented Dec 16, 2019 •

edited

Loading

humitos commented Dec 17, 2019

humitos commented Jan 13, 2020 •

edited

Loading

ericholscher left a comment

humitos commented Jan 16, 2020

Serve non-html at documentation domain though El Proxito #6419

Serve non-html at documentation domain though El Proxito #6419

Conversation

humitos commented Nov 28, 2019 • edited Loading

ericholscher left a comment

Choose a reason for hiding this comment

ericholscher commented Dec 2, 2019

davidfischer left a comment

Choose a reason for hiding this comment

humitos commented Dec 3, 2019

davidfischer commented Dec 3, 2019

ericholscher commented Dec 4, 2019

ericholscher left a comment

Choose a reason for hiding this comment

ericholscher commented Dec 16, 2019 • edited Loading

humitos commented Dec 17, 2019

humitos commented Jan 13, 2020 • edited Loading

ericholscher left a comment

Choose a reason for hiding this comment

humitos commented Jan 16, 2020

humitos commented Nov 28, 2019 •

edited

Loading

ericholscher commented Dec 16, 2019 •

edited

Loading

humitos commented Jan 13, 2020 •

edited

Loading